Skip to content

Minor cleanups FrozenIndexInput#93309

Merged
original-brownbear merged 2 commits intoelastic:mainfrom
original-brownbear:more-efficient-frozen-input
Feb 2, 2023
Merged

Minor cleanups FrozenIndexInput#93309
original-brownbear merged 2 commits intoelastic:mainfrom
original-brownbear:more-efficient-frozen-input

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

Some random finds while working with this code. We shouldn't use a Consumer instead of a LongConsumer as we never pass null to the consumer.
Also, way simplified the locking around the Lucene Bytebuffer b to simplify the code and technically make it a little faster/less-contenting as well.
Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.

Some random finds while working with this code. We shouldn't use a Consumer<Long> instead of a LongConsumer
as we never pass `null` to the consumer.
Also, way simplified the locking around the Lucene `Bytebuffer b` to simplify the code and technically make it
a little faster/less-contenting as well.
Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.
@original-brownbear original-brownbear added >non-issue :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.7.0 labels Jan 27, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Jan 27, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

} finally {
preventAsyncBufferChanges.run();
if (bufferWriteLocked == false) {
luceneByteBufPermits.acquire(Integer.MAX_VALUE);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to release here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is just a paranoid fail-safe like we had in the previous version of this, we never release. We always just acquire all of them to close the buffer for writes for good to have the same behaviour we previously had via the boolean but simpler.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. I wonder if we could have some sort of ByteBuffer wrapper that we could "invalidate" such at it then prevent any reading from it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking the PR to be merged)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see later I'd say. This is definitely a pattern we don't just have here but also in at least one other spot. I'm still hoping maybe we can find a way to not have to do this (passing the buffer around to other threads in general) since somehow this is never actually safe :) I'll think on it!

Copy link
Copy Markdown
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@original-brownbear
Copy link
Copy Markdown
Contributor Author

Thanks Tanguy!

@original-brownbear original-brownbear merged commit 03f8ea5 into elastic:main Feb 2, 2023
@original-brownbear original-brownbear deleted the more-efficient-frozen-input branch February 2, 2023 13:32
mark-vieira pushed a commit to mark-vieira/elasticsearch that referenced this pull request Feb 2, 2023
Some random finds while working with this code. We shouldn't use a Consumer<Long> instead of a LongConsumer
as we never pass `null` to the consumer.
Also, way simplified the locking around the Lucene `Bytebuffer b` to simplify the code and technically make it
a little faster/less-contenting as well.
Plus, made use of modern Java's buffer slicing to simplify the slicing of the Lucene buffer.
@original-brownbear original-brownbear restored the more-efficient-frozen-input branch April 18, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed Meta label for distributed team. v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants